feat: add new mui formik color picker component#231
Conversation
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
📝 WalkthroughWalkthroughAdds local client filtering to MuiFormikAsyncAutocomplete, implements MuiFormikColorInput (Formik-connected color TextField with debounced writes), exports the component, adds a webpack entry, and introduces DEBOUNCE_WAIT_150. ChangesAsync Select: localFilter mode
Color Input component + exports / build entry
Sequence DiagramsequenceDiagram
participant User
participant AsyncComponent
participant RemoteAPI
Note over AsyncComponent: Mode = localFilter ? local : remote
User->>AsyncComponent: Type input
alt localFilter == true
AsyncComponent->>AsyncComponent: apply filterOptions on options
AsyncComponent-->>User: show filtered options
else remote mode
AsyncComponent->>AsyncComponent: debounce input -> set searchTerm
AsyncComponent->>RemoteAPI: fetchOptions(searchTerm)
RemoteAPI-->>AsyncComponent: return options
AsyncComponent-->>User: show fetched options
end
User->>AsyncComponent: Select option
AsyncComponent-->>User: emit selection / update Formik
sequenceDiagram
participant User
participant ColorComponent
participant Formik
User->>ColorComponent: pick color
ColorComponent->>ColorComponent: update localValue and start debounce
ColorComponent->>Formik: setValue (debounced)
User->>ColorComponent: blur
ColorComponent->>Formik: onBlur and setTouched
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/mui/formik-inputs/mui-formik-async-select.js`:
- Around line 34-35: The component defines and accepts isMulti but still uses
the older multiple flag when computing Autocomplete value and onChange, causing
Formik to store the wrong shape; update all selection/value logic to
consistently use isMulti (not multiple) — pass isMulti to the MUI Autocomplete
prop, branch on isMulti when deriving value from Formik field and when calling
setFieldValue (map arrays for multi-select, single item otherwise), and
remove/rename any references to multiple so Formik and Autocomplete agree on the
value shape (check functions/variables around isMulti, Autocomplete usage, and
the onChange/value handlers).
- Around line 107-110: The local filtering callback currently calls
opt.label.toLowerCase() which will throw if opt.label is
null/undefined/non-string; update the filter used in options.filter((opt) =>
...) to coerce or guard the label and inputValue (e.g., use String(opt.label ??
'') or (opt.label || '').toString()) and similarly normalize inputValue before
calling toLowerCase(), then perform the includes check; ensure you update the
inline filter expression where options.filter is defined to be null-safe for
labels.
In `@src/components/mui/formik-inputs/mui-formik-color-input.js`:
- Line 8: The localValue state initialized with useState(field.value ||
"#000000") can become stale on form resets; add an effect that listens for
changes to field.value (the Formik value) and calls setLocalValue(field.value ??
"#000000") to keep the color swatch in sync; locate the localValue and
setLocalValue declarations in mui-formik-color-input.js and add a useEffect
referencing field.value to update localValue when Formik reinitializes or
resets.
- Around line 20-34: The TextField currently spreads {...rest} last which allows
callers to override controlled props (type, value, onChange, onBlur) and bypass
Formik; update the component that renders TextField so you either spread
{...rest} first (before value={localValue}, onChange={handleChange},
onBlur={handleBlur}, type="color") or, preferably, strip conflicting keys from
rest (delete or omit 'type','value','onChange','onBlur') before spreading into
TextField; ensure the retained props are localValue, handleChange, handleBlur,
and the type remains "color" so Formik binding (meta.touched/meta.error) cannot
be overridden.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5aedc0a-4634-4a6b-9aa5-2640867c3e16
📒 Files selected for processing (2)
src/components/mui/formik-inputs/mui-formik-async-select.jssrc/components/mui/formik-inputs/mui-formik-color-input.js
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
…ments Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/components/mui/formik-inputs/mui-formik-async-select.js (2)
115-118:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
opt.label.toLowerCase()still throws on null/undefined labels.The
filterOptionscallback can crash if any option has a missing label. This was flagged in a previous review and remains unresolved.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/mui/formik-inputs/mui-formik-async-select.js` around lines 115 - 118, The filterOptions callback in mui-formik-async-select.js can throw when an option has a null/undefined label; update the filter function used by filterOptions (the arrow function in that file) to defensively coerce the label to a string before calling toLowerCase (e.g., use (opt && (opt.label ?? '')) or String(opt?.label ?? '') then toLowerCase) and also guard against opt being falsy, so options with missing labels no longer crash the filtering logic.
42-43:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
isMulti/multipleduality persists —AutocompleteandhandleChangestill disagree on multi-select shape.Line 107 passes
multiple={isMulti}toAutocomplete, whilehandleChange(line 84) still branches onmultiple. WhenisMulti=truebutmultiple=false(the default), Formik receives a single value while MUI is in multi-select mode, producing a broken experience.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/mui/formik-inputs/mui-formik-async-select.js` around lines 42 - 43, The component mixes two names for the same concept: use the single prop name isMulti everywhere to avoid shape mismatches. Update the handleChange function to branch on isMulti (not multiple), ensure the Autocomplete is passed multiple={isMulti}, and remove/normalize any use of a separate multiple prop in the component props/destructuring so Formik always receives the value shape that matches Autocomplete's multiple={isMulti} behavior.
🧹 Nitpick comments (1)
src/components/mui/formik-inputs/mui-formik-async-select.js (1)
158-164: ⚡ Quick winPropTypes block is incomplete — 6 of 11 props are undocumented.
The following props accepted by the component are absent from the
propTypesdefinition:multiple,placeholder,plainValue,hiddenOptions,formatSelectedValue, andisMulti. MissingisMultiis especially risky given the existingisMulti/multipleconfusion, as dev-mode warnings won't catch type mismatches on either prop.♻️ Proposed completion
MuiFormikAsyncAutocomplete.propTypes = { name: PropTypes.string.isRequired, queryFunction: PropTypes.func.isRequired, formatOption: PropTypes.func, + formatSelectedValue: PropTypes.func, queryParams: PropTypes.array, + hiddenOptions: PropTypes.array, + multiple: PropTypes.bool, + isMulti: PropTypes.bool, + plainValue: PropTypes.bool, localFilter: PropTypes.bool, + placeholder: PropTypes.string, };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/mui/formik-inputs/mui-formik-async-select.js` around lines 158 - 164, The propTypes for MuiFormikAsyncAutocomplete are missing six props (multiple, placeholder, plainValue, hiddenOptions, formatSelectedValue, isMulti); update the MuiFormikAsyncAutocomplete.propTypes object to include these properties with appropriate types (multiple: PropTypes.bool, isMulti: PropTypes.bool, placeholder: PropTypes.string, plainValue: PropTypes.bool, hiddenOptions: PropTypes.array, formatSelectedValue: PropTypes.func) so React dev-mode warnings catch misuse, and ensure the component’s defaultProps (if any) and internal logic treat isMulti and multiple consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/components/mui/formik-inputs/mui-formik-async-select.js`:
- Around line 115-118: The filterOptions callback in mui-formik-async-select.js
can throw when an option has a null/undefined label; update the filter function
used by filterOptions (the arrow function in that file) to defensively coerce
the label to a string before calling toLowerCase (e.g., use (opt && (opt.label
?? '')) or String(opt?.label ?? '') then toLowerCase) and also guard against opt
being falsy, so options with missing labels no longer crash the filtering logic.
- Around line 42-43: The component mixes two names for the same concept: use the
single prop name isMulti everywhere to avoid shape mismatches. Update the
handleChange function to branch on isMulti (not multiple), ensure the
Autocomplete is passed multiple={isMulti}, and remove/normalize any use of a
separate multiple prop in the component props/destructuring so Formik always
receives the value shape that matches Autocomplete's multiple={isMulti}
behavior.
---
Nitpick comments:
In `@src/components/mui/formik-inputs/mui-formik-async-select.js`:
- Around line 158-164: The propTypes for MuiFormikAsyncAutocomplete are missing
six props (multiple, placeholder, plainValue, hiddenOptions,
formatSelectedValue, isMulti); update the MuiFormikAsyncAutocomplete.propTypes
object to include these properties with appropriate types (multiple:
PropTypes.bool, isMulti: PropTypes.bool, placeholder: PropTypes.string,
plainValue: PropTypes.bool, hiddenOptions: PropTypes.array, formatSelectedValue:
PropTypes.func) so React dev-mode warnings catch misuse, and ensure the
component’s defaultProps (if any) and internal logic treat isMulti and multiple
consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 043e6bbb-fe41-4edc-962a-955e38345598
📒 Files selected for processing (4)
src/components/index.jssrc/components/mui/formik-inputs/mui-formik-async-select.jssrc/components/mui/formik-inputs/mui-formik-color-input.jswebpack.common.js
✅ Files skipped from review due to trivial changes (1)
- src/components/index.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/mui/formik-inputs/mui-formik-color-input.js
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
smarcet
left a comment
There was a problem hiding this comment.
Deep review — 7 findings (2 HIGH, 3 MEDIUM, 2 LOW). Inline comments on the relevant lines below. One non-inline note: no tests are added for either new behavior — localFilter (gated effect + filterOptions branch) and the color input (debounce + blur-flush + touched-on-blur) are stateful enough to regress silently. Project already has Jest + RTL set up; suggest one minimal RTL test per component.
|
|
||
| const MuiFormikColorInput = ({ name, ...rest }) => { | ||
| const [field, meta, helpers] = useField(name); | ||
| const [localValue, setLocalValue] = useState(field.value || "#000000"); |
There was a problem hiding this comment.
[HIGH] Default "#000000" masks falsy initialValues.
useState(field.value || "#000000") silently shows black while Formik state stays "" / null. If the user never touches the input, the form submits with the original empty value while the swatch implies #000000 was selected → data mismatch on save.
Suggested fix: also seed Formik on mount when field.value is empty:
useEffect(() => { if (!field.value) helpers.setValue("#000000"); }, []);or render the swatch only when field.value is set.
There was a problem hiding this comment.
The color picker would pick a default color if the user don't interact with the component. It looks like this contradicts #231 (comment).
Please let me know if I should add some initial state to display the input as not selected since the input type="color" doesn't support an empty value
| const MuiFormikColorInput = ({ name, ...rest }) => { | ||
| const [field, meta, helpers] = useField(name); | ||
| const [localValue, setLocalValue] = useState(field.value || "#000000"); | ||
| const debounceRef = useRef(null); |
There was a problem hiding this comment.
[MEDIUM] No re-sync with external field.value changes.
localValue is initialized once from field.value and only mutated by user input. After Formik resetForm(), parent setFieldValue, or async-loaded initial values, the swatch keeps showing the old color while Formik state has changed → broken edit/reset flows.
Suggested fix:
useEffect(() => {
if (field.value !== localValue) setLocalValue(field.value || "#000000");
}, [field.value]);(guard against overwriting in-flight typing if needed.)
[LOW] No debounce cleanup on unmount.
A pending setTimeout can fire helpers.setValue after unmount → stale writes / dev warnings, especially in tabbed forms. Add:
useEffect(() => () => { if (debounceRef.current) clearTimeout(debounceRef.current); }, []);|
|
||
| useEffect(() => { | ||
| if (searchTerm) { | ||
| if (!localFilter && searchTerm) { |
There was a problem hiding this comment.
[HIGH] localFilter mode filters against whatever queryFunction("") returns — usually a paged subset.
The new docblock promises "fetches once on mount" + client-side filtering, but the only mount-time call (fetchOptions("") a few lines below) uses a queryFunction that elsewhere in this codebase honors server-side pagination/filter on input. Consumers will silently see an incomplete option list with no warning, contradicting the user-visible promise of "filter all options locally."
Suggested fix: in localFilter mode, require an explicit fetchAll/pageSize prop (or accept a pre-resolved options prop) and document the contract; otherwise emit a runtime warning.
There was a problem hiding this comment.
This can be set on the implementation as part of the queryParams according to the queryFunction used.
Since the order of the per_page parameter is not always in the same position is a bit difficult to use a default MAX_PAGE_SIZE for all queryActions
| localFilter | ||
| ? (options, { inputValue }) => | ||
| options.filter((opt) => | ||
| opt.label.toLowerCase().includes(inputValue.toLowerCase()) |
There was a problem hiding this comment.
[MEDIUM] opt.label.toLowerCase() / inputValue.toLowerCase() are unguarded.
The rest of the file already defends against missing labels (option.label || "" in getOptionLabel). One bad row from the API (label null/non-string) and the whole dropdown crashes. CodeRabbit flagged the same thing; not addressed.
Suggested fix:
(options, { inputValue }) =>
options.filter((opt) =>
String(opt.label ?? "").toLowerCase().includes(
String(inputValue ?? "").toLowerCase()
)
)| formatSelectedValue = null, | ||
| queryParams = [], | ||
| isMulti = false | ||
| isMulti = false, |
There was a problem hiding this comment.
[MEDIUM] Pre-existing multiple vs isMulti dual-prop divergence (worth fixing while editing this file).
multiple(defaultfalse, line 35) drives thehandleChangevalue-shape branch (single vs array).multiple={isMulti}(further down) is what's passed to MUIAutocomplete.
A consumer passing only isMulti={true} (without multiple={true}) gets multi-select UI with single-value Formik writes → array/object collisions on submit. The new propTypes block cements this two-name API into the public surface.
Suggested fix: collapse to one prop (multiple), or alias one to the other in destructuring (multiple = isMulti), and update propTypes accordingly.
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/mui/formik-inputs/mui-formik-async-select.js (1)
68-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemote mode keeps stale results when input is cleared.
In remote mode, fetches only happen for truthy
searchTerm, so clearing the field does not refresh to default/empty-query options and leaves prior results on screen.Suggested fix
useEffect(() => { - if (!localFilter && searchTerm) { - const delayDebounce = setTimeout(() => { - fetchOptions(searchTerm); - }, DEBOUNCE_WAIT_250); - return () => clearTimeout(delayDebounce); - } -}, [searchTerm]); + if (localFilter) return; + const delayDebounce = setTimeout(() => { + fetchOptions(searchTerm ?? ""); + }, DEBOUNCE_WAIT_250); + return () => clearTimeout(delayDebounce); +}, [localFilter, searchTerm]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/mui/formik-inputs/mui-formik-async-select.js` around lines 68 - 75, The effect only calls fetchOptions when searchTerm is truthy, leaving stale remote results when the input is cleared; update the useEffect that references localFilter, searchTerm, fetchOptions and DEBOUNCE_WAIT_250 so that when localFilter is falsy and searchTerm is falsy it triggers a refresh for the empty/default query (e.g., call fetchOptions('') or fetchOptions(undefined)) instead of doing nothing; keep the debounce behavior for non-empty searchTerm but invoke the empty-query refresh immediately (or with its own short debounce) and ensure the clearTimeout cleanup remains in place.
♻️ Duplicate comments (1)
src/components/mui/formik-inputs/mui-formik-color-input.js (1)
42-57:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent
...restfrom overriding controlled Formik props.
...restis still spread last on Line 56, so callers can overridevalue,onChange,onBlur,type, ornameand break Formik binding.Proposed fix
+ const { + type: _type, + value: _value, + onChange: _onChange, + onBlur: _onBlur, + name: _name, + ...safeRest + } = rest; + return ( <TextField + {...safeRest} type="color" name={field.name} value={localValue} onChange={handleChange} onBlur={handleBlur} @@ - {...rest} /> );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/mui/formik-inputs/mui-formik-color-input.js` around lines 42 - 57, The TextField spreads {...rest} last which allows callers to override Formik-controlled props (value, onChange, onBlur, name, type); change the render so controlled props from Formik take precedence by moving {...rest} before the explicit props or by explicitly picking allowed passthrough props and spreading only those (update the JSX using TextField, localValue, handleChange, handleBlur, meta, and field to ensure value={localValue}, onChange={handleChange}, onBlur={handleBlur}, name={field.name}, and type="color" cannot be overridden).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/mui/formik-inputs/mui-formik-color-input.js`:
- Around line 16-19: The cleanup in the useEffect currently writes to Formik
(reads field.value and calls helpers.setValue("#000000")) using a stale closure;
remove the conditional Formik write from the cleanup so it no longer mutates
Formik state on unmount—keep only the debounce timeout cleanup
(clearTimeout(debounceRef.current)) in the useEffect cleanup. Locate the
useEffect that references field.value, helpers.setValue, and debounceRef and
delete the helpers.setValue branch, leaving the debounceRef.current clear call
intact (handleBlur already flushes debounced updates).
---
Outside diff comments:
In `@src/components/mui/formik-inputs/mui-formik-async-select.js`:
- Around line 68-75: The effect only calls fetchOptions when searchTerm is
truthy, leaving stale remote results when the input is cleared; update the
useEffect that references localFilter, searchTerm, fetchOptions and
DEBOUNCE_WAIT_250 so that when localFilter is falsy and searchTerm is falsy it
triggers a refresh for the empty/default query (e.g., call fetchOptions('') or
fetchOptions(undefined)) instead of doing nothing; keep the debounce behavior
for non-empty searchTerm but invoke the empty-query refresh immediately (or with
its own short debounce) and ensure the clearTimeout cleanup remains in place.
---
Duplicate comments:
In `@src/components/mui/formik-inputs/mui-formik-color-input.js`:
- Around line 42-57: The TextField spreads {...rest} last which allows callers
to override Formik-controlled props (value, onChange, onBlur, name, type);
change the render so controlled props from Formik take precedence by moving
{...rest} before the explicit props or by explicitly picking allowed passthrough
props and spreading only those (update the JSX using TextField, localValue,
handleChange, handleBlur, meta, and field to ensure value={localValue},
onChange={handleChange}, onBlur={handleBlur}, name={field.name}, and
type="color" cannot be overridden).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dbbbb97c-f6f4-47b4-8293-5979a0a5fd99
📒 Files selected for processing (3)
src/components/mui/formik-inputs/mui-formik-async-select.jssrc/components/mui/formik-inputs/mui-formik-color-input.jssrc/utils/constants.js
✅ Files skipped from review due to trivial changes (1)
- src/utils/constants.js
| useEffect(() => () => { | ||
| if (!field.value) helpers.setValue("#000000"); | ||
| if (debounceRef.current) clearTimeout(debounceRef.current); | ||
| }, []); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/components/mui/formik-inputs/mui-formik-color-input.jsRepository: OpenStackweb/openstack-uicore-foundation
Length of output: 2319
Remove Formik writes from unmount cleanup (stale-value side effect).
On line 17, the cleanup callback captures field.value in a closure with an empty dependency array, causing it to always reference the initial value from mount rather than the current value. If the field starts with a falsy value, the cleanup will always attempt to call helpers.setValue("#000000") during unmount, even if the field value has since changed. This mutates Formik state during component teardown, which can cause unexpected side effects during navigation or conditional rendering.
The debounce timeout cleanup on line 18 is necessary and should remain, but the conditional write is redundant—handleBlur already flushes pending debounces with the correct value.
Proposed fix
- useEffect(() => () => {
- if (!field.value) helpers.setValue("#000000");
- if (debounceRef.current) clearTimeout(debounceRef.current);
- }, []);
+ useEffect(
+ () => () => {
+ if (debounceRef.current) clearTimeout(debounceRef.current);
+ },
+ []
+ );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/mui/formik-inputs/mui-formik-color-input.js` around lines 16 -
19, The cleanup in the useEffect currently writes to Formik (reads field.value
and calls helpers.setValue("#000000")) using a stale closure; remove the
conditional Formik write from the cleanup so it no longer mutates Formik state
on unmount—keep only the debounce timeout cleanup
(clearTimeout(debounceRef.current)) in the useEffect cleanup. Locate the
useEffect that references field.value, helpers.setValue, and debounceRef and
delete the helpers.setValue branch, leaving the debounceRef.current clear call
intact (handleBlur already flushes debounced updates).
ref: https://app.clickup.com/t/86b9n7p5w
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit